Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mirror: Partial atmos refactor #312

Conversation

SimpleStation14
Copy link
Member

Mirror of PR #22521: Partial atmos refactor from space-wizards space-wizards/space-station-14

18a35e7e83b2b71ee84b054d44d9ed5e595dd618

PR opened by ElectroJr at 2023-12-15 03:45:42 UTC


PR changed 43 files with 891 additions and 635 deletions.

The PR had the following labels:

  • Status: Needs Review

Original Body

This PR reworks how some parts of atmos code work. Originally it was just meant to be a performance and bugfix PR, but it has ballooned in scope. I'm not sure about some of my changes largely because I'm not sure if some things were an oversight or an intentional decision for some reason.

List of changes:

  • The MolesArchived float[] field is now read-only
    • It simply gets zeroed whenever the GasMixture is set to null instead of constantly reallocating
  • Airtight query information is now cached in TileAtmosphere
    • This means that it should only iterate over anchored entities once per update.
    • Previously an invalidated atmos tile would cause ProcessRevalidate() to query airtight entities on the same tile six times by calling a combination of GridIsTileAirBlocked(), NeedsVacuumFixing(), and GridIsTileAirBlocked(). So this should help significantly reduce component lookups & entity enumeration.
    • This does change some behaviour. In particular blocked directions are now only updated if the tile was invalidated prior to the current atmos-update, and will only ever be updated once per atmos-update.
    • AFAIK this only has an effect if the invalid tile processing is deferred over multiple ticks, and I don't think it should cause any issues?
  • Fixes a potential bug, where tiles might not dispose of their excited group if their direction flags changed.
  • MapAtmosphereComponent.Mixture is now always immutable and no longer nullable
    • I'm not sure why the mixture was nullable before? AFAICT the component is meaningless if its null?
    • Space "gas" was always immutable, but there was nothing that required planet atmospheres to be immutable. Requiring that it be immutable gets rid of the constant gas mixture cloning.
    • I don't know if there was a reason for why they weren't immutable to begin with.
  • Fixes lungs removing too much air from a gas mixture, resulting in negative moles.
  • GasMixture.Moles is now [Access] restricted to the atmosphere system.
    • This is to prevent people from improperly modifying the gas mixtures (e.g., lungs), or accidentally modifying immutable mixtures.
  • Fixes an issue where non-grid atmosphere tiles would fail to update their adjacent tiles, resulting in null reference exception spam
    • Fixes #21732
    • Fixes #21210 (probably)
  • Disconnected atmosphere tiles, i.e., tiles that aren't on or adjacent to a grid tile, will now get removed from the tile set. Previously the tile set would just always increase, with tiles never getting removed.
  • Removes various redundant component and tile-definition queries.
  • Removes some method events in favour of just using methods.
  • Map-exposded tiles now get updated when a map's atmosphere changes (or the grid moves across maps).
  • Adds a setmapatmos command for adding map-wide atmospheres.
  • Fixed (non-planet) map atmospheres rendering over grids.

Media

This PR also includes changes to the atmos debug overlay, though I've also split that off into a separate PR to make reviewing easier (#22520).

Below is a video showing that atmos still seems to work, and that trimming of disconnected tiles works:

new.mp4

For comparison, here is a video showing how current master works (disconnected tiles never get removed):

master.mp4

🆑

  • fix: Fixed a bug where partially airtight entities (e.g., thin windows or doors) could let air leak out into space.

@SimpleStation14 SimpleStation14 added the Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot label Apr 22, 2024
Copy link
Contributor

@DangerRevolution DangerRevolution left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm sure VM can review this

@SimpleStation14 SimpleStation14 marked this pull request as draft May 4, 2024 21:12
@VMSolidus VMSolidus marked this pull request as ready for review May 5, 2024 19:31
@VMSolidus
Copy link
Member

I had to double check that my own rework to Space Wind was still compatible with this, and luckily it is since I touched a function that isn't touched by this PR. The two of these combined will hopefully be an extremely significant improvement in atmos performance.

@github-actions github-actions bot added the Status: Needs Review Someone please review this label May 10, 2024
@DangerRevolution DangerRevolution added Changes: C# Changes any cs files Priority: 2-High Needs to be resolved as soon as possible Size: 2-Large For large issues/PRs Status: Needs Discussion Must be discussed Type: Rework Large changes to a system, like a mix between the Balancing, Codebase, and Respace labels labels May 10, 2024
Copy link
Member

@VMSolidus VMSolidus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is quite well made. It still isn't a theoretically perfect atmos, but it this PR represents several substantial improvements to performance into an otherwise very poorly performing system. I am excited to eventually try running this with my Space Wind Rework.

@stellar-novas
Copy link
Contributor

This PR introduced a bug that was patched in space-wizards/space-station-14#26441. We should either modify this PR to include the fix, or merge the fix PR at the same time, so downstreams don't get stuck with bugged atmos for a few days.

@VMSolidus
Copy link
Member

Yep, thanks for the reminder about this. Let's not merge this until I also am ready to immediately afterwards merge space-wizards/space-station-14#26441

@VMSolidus
Copy link
Member

I need this merged before I can finish the extremely urgent engine PR. I'll include the fixes for this in the engine PR...
#416

@VMSolidus VMSolidus mentioned this pull request May 20, 2024
Copy link
Member

@DEATHB4DEFEAT DEATHB4DEFEAT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have the time to look over this in detail, but it looks fine.

@VMSolidus VMSolidus merged commit 6e0ffe8 into Simple-Station:master May 20, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Priority: 2-High Needs to be resolved as soon as possible Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot Size: 2-Large For large issues/PRs Status: Needs Discussion Must be discussed Status: Needs Review Someone please review this Type: Rework Large changes to a system, like a mix between the Balancing, Codebase, and Respace labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants